sdl_wrapper/sdl_wrapper.pipeline.cpp (1)

128-128: Consider using named constants for uniform buffer counts.

The hardcoded values 1 and 0 passed as the third parameter to create_shader are not self-documenting. Using named constants would improve readability and maintainability.

Consider defining constants at the class or namespace level:

// At class or namespace scope
static constexpr uint32_t VERTEX_UNIFORM_BUFFER_COUNT = 1;
static constexpr uint32_t FRAGMENT_UNIFORM_BUFFER_COUNT = 0;

Then use them in the calls:

-            m_vertex_shader = m_device->create_shader(code, SDL_GPU_SHADERSTAGE_VERTEX, 1);
+            m_vertex_shader = m_device->create_shader(code, SDL_GPU_SHADERSTAGE_VERTEX, VERTEX_UNIFORM_BUFFER_COUNT);

-            m_fragment_shader = m_device->create_shader(code, SDL_GPU_SHADERSTAGE_FRAGMENT, 0);
+            m_fragment_shader = m_device->create_shader(code, SDL_GPU_SHADERSTAGE_FRAGMENT, FRAGMENT_UNIFORM_BUFFER_COUNT);

Also applies to: 157-157

main.cpp (3)

94-101: Consider simplifying identity matrix initialization.

The manual identity matrix initialization is correct but verbose. You could simplify it with standard library utilities or a helper function.

Example using std::fill:

std::fill(cam.m.begin(), cam.m.end(), 0.0F);
cam.m[0] = cam.m[5] = cam.m[10] = cam.m[15] = 1.0F;

Or create a reusable helper:

static void setIdentity(std::array<float, 16>& mat) {
    std::fill(mat.begin(), mat.end(), 0.0F);
    mat[0] = mat[5] = mat[10] = mat[15] = 1.0F;
}

---

262-288: Consider extracting matrix operations for reusability.

The view matrix computation is mathematically correct, but the inline lambda for matrix multiplication adds complexity to the draw function. Extracting it would improve readability and enable reuse.

Consider moving matrix operations to helper functions:

// At class or namespace scope
static void multiply4x4(const float* A, const float* B, float* result) {
    for (int col = 0; col < 4; ++col) {
        for (int row = 0; row < 4; ++row) {
            result[col * 4 + row] = 
                A[0 * 4 + row] * B[col * 4 + 0] +
                A[1 * 4 + row] * B[col * 4 + 1] +
                A[2 * 4 + row] * B[col * 4 + 2] +
                A[3 * 4 + row] * B[col * 4 + 3];
        }
    }
}

static void makeRotationY(float angle, float* matrix) {
    float c = std::cos(angle);
    float s = std::sin(angle);
    float temp[16] = {
        c, 0.0f, s, 0.0f,
        0.0f, 1.0f, 0.0f, 0.0f,
        -s, 0.0f, c, 0.0f,
        0.0f, 0.0f, 0.0f, 1.0f
    };
    std::copy(std::begin(temp), std::end(temp), matrix);
}

This would make the draw function more readable while keeping the matrix math testable and reusable.

---

337-356: Input handling looks good; consider angle normalization for long sessions.

The keyboard controls correctly update yaw and pitch angles. The implementation is straightforward and functional.

For very long sessions, unbounded angle accumulation could lead to floating-point precision loss. Consider normalizing angles to the range [0, 2π) or [-π, π):

// After updating yaw or pitch:
yaw = std::fmod(yaw, 2.0f * M_PI);
pitch = std::fmod(pitch, 2.0f * M_PI);

Or clamp pitch to prevent gimbal lock:

pitch = std::clamp(pitch, -M_PI/2.0f + 0.01f, M_PI/2.0f - 0.01f);

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.273Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, null pointer validation for the GpuWrapper::create_buffer method (checking m_device, SDL_CreateGPUBuffer return value, and shared_from_this() validity) is deferred to a future issue and should not be flagged in reviews.

Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:73-76
Timestamp: 2025-11-14T00:45:09.377Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, validation for m_window in the GpuWrapper::get_texture_formate() method (checking if m_window is nullptr before calling SDL_GetGPUSwapchainTextureFormat) is deferred to a future window wrapper implementation and should not be flagged in reviews.

Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.714Z
Learning: In sdl_wrapper/sdl_wrapper.buffer.ixx, null pointer validation for the BufferWrapper constructor (checking p_gpu, p_gpu->data(), and SDL_CreateGPUBuffer return value) is deferred to a future issue and should not be flagged in reviews.

main.cpp (2)

94-101: Consider a more maintainable identity matrix initialization.

The manual element-by-element initialization is correct but verbose and error-prone.

Apply this diff to use a clearer approach:

-        {
-            // identity
-            cam.m[0] = cam.m[5] = cam.m[10] = cam.m[15] = 1.0F;
-            cam.m[1] = cam.m[2] = cam.m[3] = 0.0F;
-            cam.m[4] = cam.m[6] = cam.m[7] = 0.0F;
-            cam.m[8] = cam.m[9] = cam.m[11] = 0.0F;
-            cam.m[12] = cam.m[13] = cam.m[14] = 0.0F;
-        }
+        // Initialize camera to identity matrix
+        std::fill(cam.m.begin(), cam.m.end(), 0.0F);
+        cam.m[0] = cam.m[5] = cam.m[10] = cam.m[15] = 1.0F;

---

262-288: Optimize view matrix computation.

The mulMat4 lambda is defined every frame, adding unnecessary overhead. Additionally, the view matrix is recomputed every frame even when yaw and pitch haven't changed.

Consider these improvements:

1. Move the lambda to class scope or use a static helper function:

Add to the class:

static void mulMat4(const float* A, const float* B, float* C)
{
    for (int col = 0; col < 4; ++col)
    {
        for (int row = 0; row < 4; ++row)
        {
            C[col * 4 + row] = A[0 * 4 + row] * B[col * 4 + 0] + A[1 * 4 + row] * B[col * 4 + 1] +
                A[2 * 4 + row] * B[col * 4 + 2] + A[3 * 4 + row] * B[col * 4 + 3];
        }
    }
}

2. Track if camera changed and only recompute when needed:

Add a member: bool camera_dirty = true;

In event(): Set camera_dirty = true; after modifying yaw or pitch.

In draw(): Only recompute if camera_dirty, then set it to false.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:0-0
Timestamp: 2025-11-13T02:58:26.859Z
Learning: The sdl_wrapper library (in the repository WSQS/sdl_test) does not allow throwing exceptions. Error handling must use return codes, validity checks, or other non-exception mechanisms.

Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.273Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, null pointer validation for the GpuWrapper::create_buffer method (checking m_device, SDL_CreateGPUBuffer return value, and shared_from_this() validity) is deferred to a future issue and should not be flagged in reviews.

Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.714Z
Learning: In sdl_wrapper/sdl_wrapper.buffer.ixx, null pointer validation for the BufferWrapper constructor (checking p_gpu, p_gpu->data(), and SDL_CreateGPUBuffer return value) is deferred to a future issue and should not be flagged in reviews.

Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:73-76
Timestamp: 2025-11-14T00:45:09.377Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, validation for m_window in the GpuWrapper::get_texture_formate() method (checking if m_window is nullptr before calling SDL_GetGPUSwapchainTextureFormat) is deferred to a future window wrapper implementation and should not be flagged in reviews.

main.cpp (2)

265-291: Verify rotation matrix correctness and consider extraction.

The rotation matrices and multiplication logic should be verified to ensure correct camera behavior:

1. Confirm that the matrix element ordering and signs produce the intended rotation (yaw around Y, pitch around X).
2. Verify that the multiplication order Rx * Ry achieves the desired pitch-then-yaw transformation.
3. Consider whether a view matrix should also include translation (camera position), or if rotation-only is intentional.

Consider extracting the matrix multiplication lambda and rotation matrix construction into named helper functions to improve readability and reusability:

+    auto createRotationY = [](float angle) -> std::array<float, 16> {
+        const float c = std::cos(angle);
+        const float s = std::sin(angle);
+        return {c, 0.0F, -s, 0.0F,
+                0.0F, 1.0F, 0.0F, 0.0F,
+                s, 0.0F, c, 0.0F,
+                0.0F, 0.0F, 0.0F, 1.0F};
+    };
+
+    auto createRotationX = [](float angle) -> std::array<float, 16> {
+        const float c = std::cos(angle);
+        const float s = std::sin(angle);
+        return {1.0F, 0.0F, 0.0F, 0.0F,
+                0.0F, c, s, 0.0F,
+                0.0F, -s, c, 0.0F,
+                0.0F, 0.0F, 0.0F, 1.0F};
+    };
+
+    auto multiplyMat4 = [](const std::array<float, 16>& A, const std::array<float, 16>& B) -> std::array<float, 16> {
+        std::array<float, 16> C{};
+        for (int col = 0; col < 4; ++col) {
+            for (int row = 0; row < 4; ++row) {
+                C[col * 4 + row] = A[0 * 4 + row] * B[col * 4 + 0] + 
+                                   A[1 * 4 + row] * B[col * 4 + 1] +
+                                   A[2 * 4 + row] * B[col * 4 + 2] + 
+                                   A[3 * 4 + row] * B[col * 4 + 3];
+            }
+        }
+        return C;
+    };

---

345-356: Consider using named constants for rotation increments.

The magic number 0.1f is used for rotation speed. While functional, consider extracting this to a named constant for clarity and easier tuning.

+    static constexpr float ROTATION_SPEED = 0.1F;
+
     if (event->type == SDL_EVENT_KEY_DOWN)
     {
         switch (event->key.key)
         {
         case SDLK_UP:
-            pitch += 0.1f;
+            pitch += ROTATION_SPEED;
             pitch = std::clamp<float>(pitch, -M_PI_2, M_PI_2);
             break;
         case SDLK_DOWN:
-            pitch -= 0.1f;
+            pitch -= ROTATION_SPEED;
             pitch = std::clamp<float>(pitch, -M_PI_2, M_PI_2);
             break;
         case SDLK_LEFT:
-            yaw += 0.1f;
+            yaw += ROTATION_SPEED;
             break;
         case SDLK_RIGHT:
-            yaw -= 0.1f;
+            yaw -= ROTATION_SPEED;
             break;

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:0-0
Timestamp: 2025-11-13T02:58:26.859Z
Learning: The sdl_wrapper library (in the repository WSQS/sdl_test) does not allow throwing exceptions. Error handling must use return codes, validity checks, or other non-exception mechanisms.

Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.273Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, null pointer validation for the GpuWrapper::create_buffer method (checking m_device, SDL_CreateGPUBuffer return value, and shared_from_this() validity) is deferred to a future issue and should not be flagged in reviews.

Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.714Z
Learning: In sdl_wrapper/sdl_wrapper.buffer.ixx, null pointer validation for the BufferWrapper constructor (checking p_gpu, p_gpu->data(), and SDL_CreateGPUBuffer return value) is deferred to a future issue and should not be flagged in reviews.

Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:73-76
Timestamp: 2025-11-14T00:45:09.377Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, validation for m_window in the GpuWrapper::get_texture_formate() method (checking if m_window is nullptr before calling SDL_GetGPUSwapchainTextureFormat) is deferred to a future window wrapper implementation and should not be flagged in reviews.